Deprecate AbstractLogstashTcpSocketAppender#secondaryConnectionTTL #634
Conversation
Deprecate AbstractLogstashTcpSocketAppender#secondaryConnectionTTL in a backward compatible way (emit a warning if the propery is set during configuration) fixes #633
| */ | ||
| @Deprecated | ||
| public void setSecondaryConnectionTTL(Duration secondaryConnectionTTL) { | ||
| addWarn("Setting <secondaryConnectionTTL> on the appender is deprecated, set it on the connection strategy using <preferPrimary.secondaryConnectionTTL> instead."); |
There was a problem hiding this comment.
<preferPrimary.secondaryConnectionTTL> is a little confusing, since it looks like xml, but is not the way the value would be set in xml.
Perhaps <preferPrimary><secondaryConnectionTTL> would be better? I'm looking for better ideas. (?)
There was a problem hiding this comment.
I came across this issue several times already when throwing exceptions for invalid properties for instance.
I initially though that using an XML-like name was a good idea certainly if we consider that most users will use an XML file to configure their logging system. If we follow this approach then <preferPrimary><secondaryConnectionTTL> is certainly better.
Another option is to opt for a JavaBean-style format and do something like "preferPrimary.secondaryConnectionTTL". But this looks weird to me...
In this particular case, maybe we could simply say Setting <secondaryConnectionTTL> directly on the appender is deprecated. Instead you should explicitly set the connection strategy to <preferPrimary> and set its <secondaryConnectionTTL> property to the desired value.
What ever option we choose, we should probably review the other classes and standardise on the chosen naming convention...
There was a problem hiding this comment.
Setting <secondaryConnectionTTL> directly on the appender is deprecated. Instead you should explicitly set the connection strategy to <preferPrimary> and set its <secondaryConnectionTTL> property to the desired value.
^ I like that.
What ever option we choose, we should probably review the other classes and standardise on the chosen naming convention
Agreed.
philsttr
added
the
warn/deprecation
Keep current behaviour for backward compatibility.
Emit a WARN status when the property is set.
fixes #633